Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor keystore interface into 3 different interfaces #16365

Merged
merged 11 commits into from
Apr 2, 2020
Merged

Refactor keystore interface into 3 different interfaces #16365

merged 11 commits into from
Apr 2, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Feb 17, 2020 •
edited
Loading

What does this PR do?

This PR refactor keystore interface into 3 different interfaces.

This PR introduces a new Keystore backend which is ReadOnly and retrieves passwords from k8s secrets through k8s API. In order to implement this, the basic Keystore interface is split into 3 different interfaces, Keystore (readonly), WritableKeystore (Keystore + store/write/delete), ListingKeystore (WritableKeystore + list) as proposed on #5832.

The new Keystore that retrieves data from k8s secrets is of Keystore (readonly) type.

#### Autodiscover adoption
In order to support this k8s Keystore in Autodiscover, a keystore object is attached on every event. The keystore object is either created or retrieved if it already exists. We create only one keystore per namespace and we reuse it for events of the same namespace.

Why is it important?

This is important in order to support different keystore backends, like k8s keystore.

This is important in order to support autodiscover hints being able to refer to k8s secrets for password retrieval.

How to test this PR locally

Try all the set of available commands:

  1. ./metricbeat keystore create
  2. ./metricbeat keystore add ES_PWD
  3. ./metricbeat keystore add ES_PWD --force
  4. echo "passpass" | ./metricbeat keystore add ES_PWD --stdin --for
  5. ./metricbeat keystore list
  6. ./metricbeat keystore remove ES_PWD

Create a key and consume it (Elastic Cloud auth or any other ES setting can be used as an example):

  1. ./metricbeat keystore add ES_PWD
  2. ./metricbeat -e -d "*" -E "cloud.id=testkeys:xxxx" -E "cloud.auth=\${ES_PWD}"

or

  1. ./metricbeat keystore add ES_HOST
  2. ./metricbeat -e -d "*" -E output.elasticsearch.hosts=["\${ES_HOST}"]

Related issues

@ChrsMark ChrsMark added enhancement libbeat Team:Integrations Label for the Integrations team autodiscovery Team:Platforms Label for the Integrations - Platforms team labels Feb 17, 2020
@ChrsMark ChrsMark self-assigned this Feb 17, 2020
@ChrsMark
Add k8s secrets Keystore backend
bd508d6 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@exekias
Copy link
Contributor

exekias commented Feb 20, 2020

I would recommend splitting this PR in 2:

  • refactor keystore interface into 3 different interfaces, with all the changes required to code using keystore.
  • add the new k8s keystore

@ChrsMark
Keep only keystore refactoring
8fc096b 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

Updated that, to keep only the Keystore refactoring. The implementation of k8s keystore could be retrieved by the previous commit (bd508d6).

I will open it for review to start shaping it more.
One thing that I'm not sure of is how we could better approach the Factory implementation. I thought of it like different factory methods for each different keystore backend. This is how I did it with k8s keystore at bd508d6. But proposals are more than welcome.

Also interface inheritance is something to consider, since for now I followed an approach of having "stronger" interfaces inheriting the ones with the lower privileges.

@ChrsMark ChrsMark changed the title Add k8s secrets Keystore backend Refactor keystore interface into 3 different interfaces Feb 20, 2020
@ChrsMark ChrsMark marked this pull request as ready for review February 20, 2020 10:00
@ChrsMark
left overs
5b03a56 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@exekias
Copy link
Contributor

exekias commented Feb 24, 2020

One thing that I'm not sure of is how we could better approach the Factory implementation. I thought of it like different factory methods for each different keystore backend. This is how I did it with k8s keystore at bd508d6. But proposals are more than welcome.

I would avoid a common Factory for now, as you don't need it yet? Left a comment about the existing one, but it looks to me that it's file_keystore specifc.

@ChrsMark
fixes
5100deb 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@exekias
Copy link
Contributor

exekias commented Feb 27, 2020

This is looking good, but tests seem to be failing

@ChrsMark
fixes
d379073 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

Yeap @exekias , tests were failing because the refactoring was not "healthy". I had to ensure that in all places the stores are properly checked if implement the respective interfaces. Feel free to comment on it.

urso
urso reviewed Mar 3, 2020
View reviewed changes
libbeat/keystore/file_keystore.go Outdated Show resolved Hide resolved
libbeat/keystore/file_keystore.go Outdated Show resolved Hide resolved
@ChrsMark
Copy link
Member Author

@urso @exekias any extra comments on this would be much appreciated so as to move this forward 🙂

urso
urso reviewed Apr 1, 2020
View reviewed changes
libbeat/cmd/keystore.go Outdated Show resolved Hide resolved
@ChrsMark
Code improvements
77ad58c 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@urso
Copy link

urso commented Apr 1, 2020

LGTM. Let's see what CI says.

@ChrsMark
Improve function docstrings
4f06d75 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 1, 2020

jenkins, test this again please

@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 2, 2020 •
edited
Loading

CI looks happy now. I also added manual testing notes that I used in my testing process too.

@ChrsMark ChrsMark added needs_backport PR is waiting to be backported to other branches. v7.8.0 labels Apr 2, 2020
@ChrsMark ChrsMark merged commit d99dde2 into elastic:master Apr 2, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Apr 2, 2020
@ChrsMark
Refactor keystore interface into 3 different interfaces (elastic#16365)
3c58ec6 
(cherry picked from commit d99dde2)
@ChrsMark ChrsMark mentioned this pull request Apr 2, 2020
Merged
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Apr 2, 2020
ChrsMark added a commit that referenced this pull request Apr 2, 2020
@ChrsMark
Refactor keystore interface into 3 different interfaces (#16365) (#17435
7759cb5 
)

(cherry picked from commit d99dde2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urso urso urso approved these changes

@exekias exekias exekias approved these changes

Assignees

@ChrsMark ChrsMark

Labels
autodiscovery enhancement libbeat Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team v7.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrsMark @exekias @urso @andresrc